Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use base-e (not 10) in expected_win_probability #54

Closed

Conversation

dexonsmith
Copy link
Contributor

@dexonsmith dexonsmith commented Jan 11, 2024

Change expected_win_probability to use base-e (instead of base-10), to match Glicko-2. v5_glicko2_update was already (correctly) using base-e.

This follow-up to 87c3fa8 gets handles the math domain error from tallying results on large small board handicaps. The base-10 bug made the expected win rate 1.0 (100%).

Of course, it's still possible to skip large handicap games when tallying results... but no longer necessary.

Relates to #45.

@dexonsmith dexonsmith requested a review from anoek January 11, 2024 21:12
@dexonsmith dexonsmith changed the title Use base-e (not-10) in expected_win_probability Use base-e (not 10) in expected_win_probability Jan 11, 2024
Change `expected_win_probability` to use base-e (instead of base-10), to match
Glicko-2.  `v5_glicko2_update` was already (correctly) using base-e.

This follow-up to 87c3fa8 gets handles the
math domain error from tallying results on large small board handicaps. The
base-10 bug made the expected win rate 1.0 (100%).

Of course, it's still possible to skip large handicap games when tallying
results... but no longer necessary.
@dexonsmith dexonsmith force-pushed the expected-win-probability-base-e branch from 170fee1 to 1141878 Compare January 11, 2024 21:12
/ 400
)
)
/ 400)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anoek, I'm skeptical of this / 400. Any thoughts on why it's here?

@dexonsmith
Copy link
Contributor Author

@anoek, this function expected_win_probability shows up in other repos, with the same bug, but aside from unit tests it doesn't seem to be USED by anything (except prediction_cost in this, the goratings repo).

Let me know if/when to send out parallel PRs. Personally I'd be tempted to delete it from the other repos since it's not used there but up to you.

@anoek
Copy link
Member

anoek commented Jan 11, 2024

Ah, so the source of confusion is that this function is using the original glicko expected outcome formula on page 5 of http://www.glicko.net/glicko/glicko.pdf , as opposed to from http://glicko.net/glicko/glicko2.pdf

The original formula was this:

image

so that's where the 10** and / 400 come from

Note that the actual glicko2 implementation uses the E as defined by the glicko2 paper, this method was just used for that predicted outcome tally.

I'm guessing what happened is I did the original implemenation, came back to it a year or two later, thought "Hey I want to look at the estimated outcome", searched the glicko2 paper and didn't find the keywords I wanted and didn't pause long enough to think about the math, meanwhile the original glicko paper had a function that sounded like what I wanted, and given my understanding that glicko and glicko2 are completely compatible with one another, just glicko2 updates faster/better, I probably said to myself, this should work just fine for crunching some simple analytics. And I assume it does, though I assume using E from the glicko2 paper would be an improvement.

@anoek
Copy link
Member

anoek commented Jan 11, 2024

@anoek, this function expected_win_probability shows up in other repos, with the same bug, but aside from unit tests it doesn't seem to be USED by anything (except prediction_cost in this, the goratings repo).

Let me know if/when to send out parallel PRs. Personally I'd be tempted to delete it from the other repos since it's not used there but up to you.

Yeah I reckon eliminating it is probably a good thing. Honestly I don't know that it's useful here either, it's not immediately obvious to me whether using the value like we do produces anything meaningful. Seems like if we do keep it, we should update it though.

@dexonsmith
Copy link
Contributor Author

@anoek, this function expected_win_probability shows up in other repos, with the same bug, but aside from unit tests it doesn't seem to be USED by anything (except prediction_cost in this, the goratings repo).
Let me know if/when to send out parallel PRs. Personally I'd be tempted to delete it from the other repos since it's not used there but up to you.

Yeah I reckon eliminating it is probably a good thing. Honestly I don't know that it's useful here either, it's not immediately obvious to me whether using the value like we do produces anything meaningful. Seems like if we do keep it, we should update it though.

Well, it's used in goratings to compute the prediction_cost, which is the main thing you look at, so we can't eliminate it here...

@dexonsmith
Copy link
Contributor Author

Ah, so the source of confusion is that this function is using the original glicko expected outcome formula on page 5 of http://www.glicko.net/glicko/glicko.pdf , as opposed to from http://glicko.net/glicko/glicko2.pdf

The original formula was this:

image

so that's where the 10** and / 400 come from

Note that the actual glicko2 implementation uses the E as defined by the glicko2 paper, this method was just used for that predicted outcome tally.

I'm guessing what happened is I did the original implemenation, came back to it a year or two later, thought "Hey I want to look at the estimated outcome", searched the glicko2 paper and didn't find the keywords I wanted and didn't pause long enough to think about the math, meanwhile the original glicko paper had a function that sounded like what I wanted, and given my understanding that glicko and glicko2 are completely compatible with one another, just glicko2 updates faster/better, I probably said to myself, this should work just fine for crunching some simple analytics. And I assume it does, though I assume using E from the glicko2 paper would be an improvement.

Aha. I should update it more fully then before pushing. I'll make this a draft for now.

@dexonsmith dexonsmith marked this pull request as draft January 11, 2024 22:01
@anoek
Copy link
Member

anoek commented Jan 11, 2024

Well, it's used in goratings to compute the prediction_cost, which is the main thing you look at, so we can't eliminate it here...

It's not though, the main thing I was looking at was the handicap stuff at the top, it's all about those black win rates being consistent - if you've those pegged then it means your rating to ranking map is good. Everything else is just to help validate what I was seeing, maybe develop some further intuitions, that sort of stuff. So in that regard, sure might as well update it, but the main thing is optimizing for ensuring that if we have a rank difference between players, that corresponds directly to how much of a handicap they should have.

@dexonsmith
Copy link
Contributor Author

Well, it's used in goratings to compute the prediction_cost, which is the main thing you look at, so we can't eliminate it here...

It's not though, the main thing I was looking at was the handicap stuff at the top, it's all about those black win rates being consistent - if you've those pegged then it means your rating to ranking map is good. Everything else is just to help validate what I was seeing, maybe develop some further intuitions, that sort of stuff. So in that regard, sure might as well update it, but the main thing is optimizing for ensuring that if we have a rank difference between players, that corresponds directly to how much of a handicap they should have.

Oh, the black win rate. I misunderstood which table you were looking at :/. (My fault... you did say the FIRST table, but I somehow was looking at the second.)

@dexonsmith
Copy link
Contributor Author

I think cc296a6 updates it correctly / the rest of the way. This makes the math domain error come back... so now I'm not sure it really makes a difference.

@dexonsmith
Copy link
Contributor Author

Effectively replaced this with #57 for now.

@dexonsmith dexonsmith changed the title Use base-e (not 10) in expected_win_probability Update math in 'expected_win_probability' to match 'glicko2_update' Jan 11, 2024
@dexonsmith dexonsmith changed the title Update math in 'expected_win_probability' to match 'glicko2_update' Use base-e (not 10) in expected_win_probability Jan 11, 2024
@dexonsmith
Copy link
Contributor Author

I think cc296a6 updates it correctly / the rest of the way. This makes the math domain error come back... so now I'm not sure it really makes a difference.

I'm going to close this one and re-open a clean PR to consider.

@dexonsmith dexonsmith closed this Jan 11, 2024
@dexonsmith dexonsmith deleted the expected-win-probability-base-e branch January 11, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants